Conversation
Walkthrough루틴 데이터 계층을 Flow 기반 SSOT로 전환하고 낙관적 토글·배치 동기화 로직을 도메인/데이터 계층으로 이동했습니다. 네비게이션은 타입 안전한 HomeRoute로 변경되고 하단바는 외부 상태/콜백으로 제어되며, DI에 IO 디스패처와 로컬 데이터소스 바인딩이 추가되었습니다. Changes
Sequence Diagram(s)sequenceDiagram
participant VM as HomeViewModel
participant UC as ObserveWeeklyRoutinesUseCase
participant Repo as RoutineRepository
participant Local as RoutineLocalDataSource
participant Remote as RoutineRemoteDataSource
VM->>UC: invoke(startDate, endDate)
UC->>Repo: observeWeeklyRoutines(startDate, endDate)
Repo->>Local: read routineSchedule & lastFetchRange
alt cache miss or range mismatch
Repo->>Remote: fetchWeeklyRoutines(startDate, endDate)
Remote-->>Repo: RoutineSchedule
Repo->>Local: saveSchedule(schedule, startDate, endDate)
end
Repo-->>UC: emit RoutineSchedule (from Local StateFlow)
UC-->>VM: Flow<RoutineSchedule> emission
VM->>VM: reduce state with emitted schedule
sequenceDiagram
participant VM as HomeViewModel
participant UC as ApplyRoutineToggleUseCase
participant Repo as RoutineRepository
participant Local as RoutineLocalDataSource
participant Remote as RoutineRemoteDataSource
VM->>UC: invoke(dateKey, routineId, isCompleted, states, strategy)
UC->>UC: compute RoutineCompletionInfo
UC->>Repo: applyRoutineToggle(dateKey, routineId, completionInfo)
Repo->>Local: applyOptimisticToggle(dateKey, routineId, completionInfo)
Local-->>Repo: updated StateFlow value
Repo->>Repo: record original state & enqueue pending change
Repo->>Repo: emit to debounced sync trigger (≈500ms)
par after debounce
Repo->>Remote: flushAllPendingChanges(actualChanges)
alt success
Remote-->>Repo: success
Repo->>Repo: clear pending queue
else failure
Remote-->>Repo: error
Repo->>Repo: emit syncError
Repo->>Local: clearCache()
Repo->>Remote: refetch and restore via observe flow
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (2)
data/src/main/java/com/threegap/bitnagil/data/routine/datasourceImpl/RoutineLocalDataSourceImpl.kt (1)
54-56:clearCache에서lastFetchRange초기화 누락
clearCache()가_routineSchedule만null로 설정하고lastFetchRange는 유지합니다. 이로 인해 캐시가 무효화되었지만 범위 정보는 남아있는 불일치 상태가 될 수 있습니다.♻️ lastFetchRange 초기화 추가 제안
override fun clearCache() { + lastFetchRange = null _routineSchedule.update { null } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@data/src/main/java/com/threegap/bitnagil/data/routine/datasourceImpl/RoutineLocalDataSourceImpl.kt` around lines 54 - 56, The clearCache() implementation only nulls _routineSchedule but leaves lastFetchRange stale; update RoutineLocalDataSourceImpl.clearCache to also reset the fetch-range state (e.g., set lastFetchRange to null or its initial value) at the same time you call _routineSchedule.update so the cache and its range metadata stay consistent; reference the symbols clearCache, _routineSchedule, and lastFetchRange and perform the reset using the same concurrency-safe/state-updating mechanism used for _routineSchedule.presentation/src/main/java/com/threegap/bitnagil/presentation/screen/routinelist/RoutineListViewModel.kt (1)
38-41:init블록 대신container의onCreate콜백 사용을 권장합니다.
HomeViewModel에서는container(onCreate = { ... })를 사용하여 lifecycle-aware하게 초기화하는 반면, 이 ViewModel은init블록을 사용합니다. 이로 인해 container가 구독되기 전에 flow 수집이 시작될 수 있습니다.♻️ onCreate 콜백 사용 제안
- override val container: Container<RoutineListState, RoutineListSideEffect> = container(initialState = RoutineListState.INIT) + override val container: Container<RoutineListState, RoutineListSideEffect> = container( + initialState = RoutineListState.INIT, + onCreate = { + updateDate(selectedDate) + observeWeeklyRoutines() + }, + ) - init { - updateDate(selectedDate) - observeWeeklyRoutines() - }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@presentation/src/main/java/com/threegap/bitnagil/presentation/screen/routinelist/RoutineListViewModel.kt` around lines 38 - 41, The init block in RoutineListViewModel starts work immediately (calling updateDate(selectedDate) and observeWeeklyRoutines()) and can begin collecting flows before the container is subscribed; move that initialization into the container's onCreate callback so it is lifecycle-aware: remove the init block and call updateDate(selectedDate) and observeWeeklyRoutines() from container(onCreate = { ... }) inside RoutineListViewModel (mirror the pattern used in HomeViewModel) to ensure flows are collected only after the container is created.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/src/main/java/com/threegap/bitnagil/navigation/home/HomeNavigator.kt`:
- Around line 33-37: The current navigateTo(route: HomeRoute) uses
navController.navigate(route) with popUpTo(0) which clears the entire back stack
and prevents per-tab state preservation; replace that behavior by navigating
with a popUpTo targeting the nav graph's start destination (use
navController.graph.findStartDestination().id) and enable saveState = true
inside the popUpTo block, add launchSingleTop = true on the navigate options,
and set restoreState = true so each Bottom Navigation tab preserves and restores
its own back stack and UI state instead of wiping it on every tab switch.
In
`@data/src/main/java/com/threegap/bitnagil/data/routine/datasource/RoutineLocalDataSource.kt`:
- Around line 9-13: clearCache currently only resets the in-memory schedule but
leaves lastFetchRange set, causing stale range-based cache hits; update
RoutineLocalDataSourceImpl.clearCache() so it also sets lastFetchRange = null
when you reset _routineSchedule (i.e., clear both the schedule and the
lastFetchRange fields in the clearCache implementation to maintain cache
consistency).
In
`@data/src/main/java/com/threegap/bitnagil/data/routine/datasourceImpl/RoutineLocalDataSourceImpl.kt`:
- Around line 16-22: lastFetchRange is a single mutable var causing cache
inconsistency when multiple collectors (e.g., HomeViewModel and
RoutineListViewModel) call observeWeeklyRoutines with different ranges; change
the shared single-range cache to a range-keyed cache or per-consumer tracking:
replace lastFetchRange and the single _routineSchedule state usage in
RoutineLocalDataSourceImpl with a Map<Pair<String,String>, RoutineSchedule> (or
track ranges per-collector ID) and update saveSchedule, refreshCache, and
observeWeeklyRoutines to read/write by the specific range key so concurrent
observers don’t overwrite each other’s cached ranges and data.
In
`@data/src/main/java/com/threegap/bitnagil/data/routine/repositoryImpl/RoutineRepositoryImpl.kt`:
- Around line 84-96: The rollback uses routineLocalDataSource.lastFetchRange
which can differ from the week that dateKey refers to; update
flushPendingChanges to compute or retrieve the correct rollback range for the
given dateKey instead of relying on lastFetchRange — e.g., calculate the week
range from dateKey (or maintain a map like originalFetchRangeByDate keyed by
dateKey when optimistic updates are created) and call fetchAndSave(rangeStart,
rangeEnd) with that range; ensure the code paths around pendingChangesByDate,
originalStatesByDate and the onFailure handler use that computed/retrieved range
to rollback the exact week for the dateKey.
- Around line 39-44: The mutableMapOf instances pendingChangesByDate and
originalStatesByDate in RoutineRepositoryImpl are not thread-safe and are
concurrently accessed by applyRoutineToggle (called from multiple ViewModels)
and flushPendingChanges (running on repositoryScope); replace them with
thread-safe structures or add synchronization: either change both to
ConcurrentHashMap<String, ConcurrentHashMap<String, RoutineCompletionInfo>>
(ensure inner maps are concurrent too) or introduce a Mutex used to wrap every
read/write/iteration in applyRoutineToggle and flushPendingChanges so all
accesses are protected; update any code that iterates or updates nested maps to
use thread-safe patterns (e.g., computeIfAbsent on concurrent maps or withLock
on the Mutex) to avoid ConcurrentModificationException and data loss.
- Around line 55-65: The current observeWeeklyRoutines flow mutates a single
shared routineLocalDataSource.lastFetchRange and calls
clearCache()/fetchAndSave(startDate, endDate), causing cache thrashing when
multiple subscribers request different ranges; change observeWeeklyRoutines to
provide per-range isolation by keying flows by (startDate,endDate) instead of
using the single lastFetchRange: introduce a map (e.g.,
MutableMap<Pair<String,String>, StateFlow<RoutineSchedule?>> or SharedFlow)
inside RoutineRepositoryImpl that returns or creates a dedicated flow per range,
call fetchAndSave(range) and update only that range's state (or have
routineLocalDataSource expose per-range storage/fetch methods), and remove
dependence on routineLocalDataSource.lastFetchRange/clearCache so concurrent
subscribers for different ranges do not clobber each other.
In
`@domain/src/main/java/com/threegap/bitnagil/domain/routine/usecase/ApplyRoutineToggleUseCase.kt`:
- Around line 12-17: ApplyRoutineToggleUseCase.invoke currently trusts
UI-snapshot params (isCompleted, subRoutineCompletionStates) which can be stale;
move the toggle calculation into the data layer so the final state is derived
from the source-of-truth atomically. Change ApplyRoutineToggleUseCase.invoke to
pass only identity (dateKey, routineId) and the toggle intent/strategy to
RoutineLocalDataSource (or a new method on it), and implement an atomic
read-modify-write in RoutineLocalDataSource that reads the latest completion
state, computes the new isCompleted and subRoutineCompletionStates (including
the sub-routine toggling logic), persists the result, and returns the updated
state; remove reliance on the passed snapshot params from
ApplyRoutineToggleUseCase and ensure callers send only the minimal intent.
In
`@presentation/src/main/java/com/threegap/bitnagil/presentation/screen/routinelist/RoutineListViewModel.kt`:
- Around line 73-82: The observeWeeklyRoutines function is missing the
repeatOnSubscription wrapper and error handling; wrap the flow collection inside
repeatOnSubscription { ... } (same pattern as HomeViewModel) and add a .catch {
e -> reduce { state.copy(isLoading = false) /* optionally log or signal error */
} } before collect so exceptions don't propagate and loading state is cleared;
update references in the block to observeWeeklyRoutinesUseCase(...) and the
existing reduce calls (state.copy(...)) accordingly.
---
Nitpick comments:
In
`@data/src/main/java/com/threegap/bitnagil/data/routine/datasourceImpl/RoutineLocalDataSourceImpl.kt`:
- Around line 54-56: The clearCache() implementation only nulls _routineSchedule
but leaves lastFetchRange stale; update RoutineLocalDataSourceImpl.clearCache to
also reset the fetch-range state (e.g., set lastFetchRange to null or its
initial value) at the same time you call _routineSchedule.update so the cache
and its range metadata stay consistent; reference the symbols clearCache,
_routineSchedule, and lastFetchRange and perform the reset using the same
concurrency-safe/state-updating mechanism used for _routineSchedule.
In
`@presentation/src/main/java/com/threegap/bitnagil/presentation/screen/routinelist/RoutineListViewModel.kt`:
- Around line 38-41: The init block in RoutineListViewModel starts work
immediately (calling updateDate(selectedDate) and observeWeeklyRoutines()) and
can begin collecting flows before the container is subscribed; move that
initialization into the container's onCreate callback so it is lifecycle-aware:
remove the init block and call updateDate(selectedDate) and
observeWeeklyRoutines() from container(onCreate = { ... }) inside
RoutineListViewModel (mirror the pattern used in HomeViewModel) to ensure flows
are collected only after the container is created.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 99e5ef16-a1fa-4233-9ecb-fd494c4deed2
📒 Files selected for processing (25)
app/src/main/java/com/threegap/bitnagil/di/data/CoroutineModule.ktapp/src/main/java/com/threegap/bitnagil/di/data/DataSourceModule.ktapp/src/main/java/com/threegap/bitnagil/navigation/home/HomeBottomNavigationBar.ktapp/src/main/java/com/threegap/bitnagil/navigation/home/HomeNavHost.ktapp/src/main/java/com/threegap/bitnagil/navigation/home/HomeNavigator.ktapp/src/main/java/com/threegap/bitnagil/navigation/home/HomeRoute.ktdata/src/main/java/com/threegap/bitnagil/data/di/CoroutineQualifier.ktdata/src/main/java/com/threegap/bitnagil/data/onboarding/repositoryImpl/OnBoardingRepositoryImpl.ktdata/src/main/java/com/threegap/bitnagil/data/routine/datasource/RoutineLocalDataSource.ktdata/src/main/java/com/threegap/bitnagil/data/routine/datasourceImpl/RoutineLocalDataSourceImpl.ktdata/src/main/java/com/threegap/bitnagil/data/routine/repositoryImpl/RoutineRepositoryImpl.ktdata/src/test/java/com/threegap/bitnagil/data/routine/repositoryImpl/RoutineRepositoryImplTest.ktdomain/src/main/java/com/threegap/bitnagil/domain/onboarding/model/OnBoardingRecommendRoutineEvent.ktdomain/src/main/java/com/threegap/bitnagil/domain/onboarding/repository/OnBoardingRepository.ktdomain/src/main/java/com/threegap/bitnagil/domain/onboarding/usecase/GetOnBoardingRecommendRoutineEventFlowUseCase.ktdomain/src/main/java/com/threegap/bitnagil/domain/routine/model/ToggleStrategy.ktdomain/src/main/java/com/threegap/bitnagil/domain/routine/model/WriteRoutineEvent.ktdomain/src/main/java/com/threegap/bitnagil/domain/routine/repository/RoutineRepository.ktdomain/src/main/java/com/threegap/bitnagil/domain/routine/usecase/ApplyRoutineToggleUseCase.ktdomain/src/main/java/com/threegap/bitnagil/domain/routine/usecase/GetWriteRoutineEventFlowUseCase.ktdomain/src/main/java/com/threegap/bitnagil/domain/routine/usecase/ObserveWeeklyRoutinesUseCase.ktdomain/src/main/java/com/threegap/bitnagil/domain/routine/usecase/RoutineCompletionUseCase.ktpresentation/src/main/java/com/threegap/bitnagil/presentation/screen/home/HomeViewModel.ktpresentation/src/main/java/com/threegap/bitnagil/presentation/screen/home/contract/HomeState.ktpresentation/src/main/java/com/threegap/bitnagil/presentation/screen/routinelist/RoutineListViewModel.kt
💤 Files with no reviewable changes (7)
- domain/src/main/java/com/threegap/bitnagil/domain/onboarding/model/OnBoardingRecommendRoutineEvent.kt
- domain/src/main/java/com/threegap/bitnagil/domain/routine/usecase/GetWriteRoutineEventFlowUseCase.kt
- domain/src/main/java/com/threegap/bitnagil/domain/onboarding/repository/OnBoardingRepository.kt
- domain/src/main/java/com/threegap/bitnagil/domain/onboarding/usecase/GetOnBoardingRecommendRoutineEventFlowUseCase.kt
- domain/src/main/java/com/threegap/bitnagil/domain/routine/usecase/RoutineCompletionUseCase.kt
- domain/src/main/java/com/threegap/bitnagil/domain/routine/model/WriteRoutineEvent.kt
- presentation/src/main/java/com/threegap/bitnagil/presentation/screen/home/contract/HomeState.kt
app/src/main/java/com/threegap/bitnagil/navigation/home/HomeNavigator.kt
Show resolved
Hide resolved
data/src/main/java/com/threegap/bitnagil/data/routine/datasource/RoutineLocalDataSource.kt
Show resolved
Hide resolved
...rc/main/java/com/threegap/bitnagil/data/routine/datasourceImpl/RoutineLocalDataSourceImpl.kt
Show resolved
Hide resolved
data/src/main/java/com/threegap/bitnagil/data/routine/repositoryImpl/RoutineRepositoryImpl.kt
Show resolved
Hide resolved
data/src/main/java/com/threegap/bitnagil/data/routine/repositoryImpl/RoutineRepositoryImpl.kt
Show resolved
Hide resolved
data/src/main/java/com/threegap/bitnagil/data/routine/repositoryImpl/RoutineRepositoryImpl.kt
Outdated
Show resolved
Hide resolved
domain/src/main/java/com/threegap/bitnagil/domain/routine/usecase/ApplyRoutineToggleUseCase.kt
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
data/src/main/java/com/threegap/bitnagil/data/routine/repositoryImpl/RoutineRepositoryImpl.kt (1)
62-71:⚠️ Potential issue | 🟠 Major여전히 단일 주차 캐시만 지원합니다.
단일
lastFetchRange와 공용routineSchedule하나만 쓰는 구조에서 Line 64의clearCache()가 다른 collector에게도 그대로 전파됩니다. PR 목표대로 HomeViewModel과 RoutineListViewModel이 서로 다른 주차를 동시에 구독하면 한쪽이 다른 쪽 데이터를 덮어쓰고,null을 본 기존 collector가 자기 범위를 다시 fetch하면서 cache thrashing까지 발생합니다. 아래 rollback/refresh 경로도 같은lastFetchRange에 묶여 있어 잘못된 주차를 다시 당길 수 있습니다. 범위별 캐시와 flow를 분리해야 합니다.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@data/src/main/java/com/threegap/bitnagil/data/routine/repositoryImpl/RoutineRepositoryImpl.kt` around lines 62 - 71, Current implementation uses a single shared lastFetchRange and routineSchedule so clearCache() and null emissions affect all collectors; change to a range-keyed cache/flow: replace routineLocalDataSource.lastFetchRange and routineLocalDataSource.routineSchedule with a Map<Pair<String,String>, RoutineSchedule?> (and/or a Map to StateFlow<RoutineSchedule?>) keyed by (startDate to endDate), update observeWeeklyRoutines to look up or create the per-range StateFlow for the given key (do not call global clearCache()), call a per-key clear or refresh only for that key, and ensure fetchAndSave(startDate, endDate) writes the result into the map entry/StateFlow for that key so multiple collectors can subscribe to different weeks without stomping each other.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@core/designsystem/src/main/java/com/threegap/bitnagil/designsystem/component/block/BitnagilConfirmDialog.kt`:
- Around line 24-39: The BitnagilConfirmDialog currently forwards onConfirm to
BasicAlertDialog's onDismissRequest which causes dismiss actions (back
press/outside touch) to trigger confirm; add a new parameter onDismiss: () ->
Unit (defaulting to empty or a no-op) to BitnagilConfirmDialog and pass
onDismiss to BasicAlertDialog's onDismissRequest while keeping onConfirm for the
confirm button handler (e.g., the existing onConfirm lambda used only for the
positive action). Update function signature (BitnagilConfirmDialog) and internal
call sites to use onDismissRequest = onDismiss and ensure any callers are
adjusted or rely on the default no-op to preserve backwards compatibility.
In
`@data/src/main/java/com/threegap/bitnagil/data/routine/repositoryImpl/RoutineRepositoryImpl.kt`:
- Around line 74-77: fetchAndSave currently throws exceptions (throw it) which
terminates flows and breaks Result<Unit> contracts; change fetchAndSave to
return Result<Unit> (or Result<...> as appropriate) instead of throwing, use
Result.success on success and Result.failure on failures, and remove direct
throw; update all callers—observeWeeklyRoutines, refreshCache,
flushAllPendingChanges and the .also invocations inside
deleteRoutine/registerRoutine/editRoutine/deleteRoutineForDay—to explicitly
handle the Result (log or emit an error state, convert to a failed Result<Unit>
for caller APIs, and avoid rethrowing) so the debounce collector and retry logic
aren’t interrupted and Result<Unit> APIs preserve their contract.
---
Duplicate comments:
In
`@data/src/main/java/com/threegap/bitnagil/data/routine/repositoryImpl/RoutineRepositoryImpl.kt`:
- Around line 62-71: Current implementation uses a single shared lastFetchRange
and routineSchedule so clearCache() and null emissions affect all collectors;
change to a range-keyed cache/flow: replace
routineLocalDataSource.lastFetchRange and routineLocalDataSource.routineSchedule
with a Map<Pair<String,String>, RoutineSchedule?> (and/or a Map to
StateFlow<RoutineSchedule?>) keyed by (startDate to endDate), update
observeWeeklyRoutines to look up or create the per-range StateFlow for the given
key (do not call global clearCache()), call a per-key clear or refresh only for
that key, and ensure fetchAndSave(startDate, endDate) writes the result into the
map entry/StateFlow for that key so multiple collectors can subscribe to
different weeks without stomping each other.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5319d0e7-5e79-4f2e-8d71-f28129c89017
📒 Files selected for processing (9)
core/designsystem/src/main/java/com/threegap/bitnagil/designsystem/component/block/BitnagilConfirmDialog.ktdata/src/main/java/com/threegap/bitnagil/data/routine/repositoryImpl/RoutineRepositoryImpl.ktdata/src/test/java/com/threegap/bitnagil/data/routine/repositoryImpl/RoutineRepositoryImplTest.ktdomain/src/main/java/com/threegap/bitnagil/domain/routine/repository/RoutineRepository.ktdomain/src/main/java/com/threegap/bitnagil/domain/routine/usecase/ObserveRoutineSyncErrorUseCase.ktpresentation/src/main/java/com/threegap/bitnagil/presentation/screen/home/HomeScreen.ktpresentation/src/main/java/com/threegap/bitnagil/presentation/screen/home/HomeViewModel.ktpresentation/src/main/java/com/threegap/bitnagil/presentation/screen/home/contract/HomeState.ktpresentation/src/main/java/com/threegap/bitnagil/presentation/screen/withdrawal/component/WithdrawalConfirmDialog.kt
🚧 Files skipped from review as they are similar to previous changes (3)
- presentation/src/main/java/com/threegap/bitnagil/presentation/screen/home/contract/HomeState.kt
- data/src/test/java/com/threegap/bitnagil/data/routine/repositoryImpl/RoutineRepositoryImplTest.kt
- presentation/src/main/java/com/threegap/bitnagil/presentation/screen/home/HomeViewModel.kt
...em/src/main/java/com/threegap/bitnagil/designsystem/component/block/BitnagilConfirmDialog.kt
Show resolved
Hide resolved
data/src/main/java/com/threegap/bitnagil/data/routine/repositoryImpl/RoutineRepositoryImpl.kt
Outdated
Show resolved
Hide resolved
- 날짜별로 반복 호출하던 syncRoutineCompletion API를 단일 호출로 통합하여 동기화 로직 개선
- fetchAndSave의 반환 타입을 Result<Unit>으로 변경하고 호출부에서 예외 처리(getOrThrow) 하도록 수정
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
data/src/main/java/com/threegap/bitnagil/data/routine/repositoryImpl/RoutineRepositoryImpl.kt (2)
64-72:⚠️ Potential issue | 🟠 Major주차 범위별 상태가 분리되지 않아 동시 구독이 서로를 덮어씁니다.
routineSchedule/lastFetchRange가 전역 1세트라서, 다른 범위를 구독하는 순간clearCache()와saveSchedule()가 기존 구독의 SSOT를 바로 바꾸고 이미 collect 중인 flow도 요청한 범위와 무관하게 그 새 값을 그대로 방출합니다. 실패 롤백도 현재lastFetchRange를 다시 읽기 때문에 변경이 발생한 주차가 아니라 “마지막으로 fetch된 주차”를 복구하게 됩니다. 범위별 flow/cache를 분리하지 않으면 다중 구독 시 데이터 일관성이 계속 깨집니다.Also applies to: 115-118
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@data/src/main/java/com/threegap/bitnagil/data/routine/repositoryImpl/RoutineRepositoryImpl.kt` around lines 64 - 72, The current implementation uses a single global cache and lastFetchRange, so concurrent subscriptions for different (startDate,endDate) ranges overwrite each other; change to a per-range cache/flow and per-range fetch metadata instead. Replace the single routineLocalDataSource.routineSchedule / lastFetchRange usage with a map keyed by Pair(startDate,endDate) (e.g., Map<Pair<String,String>, MutableStateFlow<RoutineSchedule?>> or store per-range entries inside RoutineLocalDataSource), update observeWeeklyRoutines to getOrCreate the flow for the requested range rather than clearing a global cache, have fetchAndSave(startDate,endDate) write only to that range’s entry, and ensure rollback restores that specific range’s previous value (not the global lastFetchRange); update calls to clearCache(), saveSchedule(), lastFetchRange to operate per-range or remove global clearing to avoid cross-range interference.
67-72:⚠️ Potential issue | 🟠 MajorLine 67과 Line 71의
getOrThrow()는 관찰 Flow를 종료시킵니다.초기 fetch나
null복구 fetch가 실패하면observeWeeklyRoutines()collector가 예외로 끝나고 이후 자동 업데이트도 끊깁니다. 실패는 별도 에러 신호로 내보내고, 관찰 자체는 계속 살아 있게 두는 편이 SSOT 계약에 더 맞습니다.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@data/src/main/java/com/threegap/bitnagil/data/routine/repositoryImpl/RoutineRepositoryImpl.kt` around lines 67 - 72, 현재 observe flow에서 fetchAndSave(...).getOrThrow()를 사용하면 초기 fetch나 onEach 복구 fetch 실패 시 예외가 throw되어 collector가 종료되므로, getOrThrow() 호출을 제거하고 실패를 예외로 던지지 않도록 변경하세요: 초기 블록과 onEach 블록 모두에서 fetchAndSave(startDate, endDate)를 runCatching/Result 처리하거나 try-catch로 감싸서 실패 시 routine 관찰을 중단시키지 않고 별도의 오류 채널(예: routineErrorFlow 또는 SharedFlow/Channel)로 에러를 emit하거나 로깅한 뒤 계속 진행하도록 만드세요; 대상 심볼: fetchAndSave, routineLocalDataSource.routineSchedule, observeWeeklyRoutines (onEach 처리 부분).
🧹 Nitpick comments (1)
data/src/test/java/com/threegap/bitnagil/data/routine/repositoryImpl/RoutineRepositoryImplTest.kt (1)
48-80: 동시 주차 구독 회귀 테스트가 아직 없습니다.여기 observe/rollback 테스트는 모두 순차 구독만 검증해서, 서로 다른 주차를 동시에 collect할 때
lastFetchRange와 단일 로컬 캐시가 충돌하는 경우를 잡지 못합니다. 같은 repository를 여러 ViewModel이 공유하는 변경이라면, 두 collector를 동시에 유지한 채 한쪽 fetch/rollback이 다른 쪽 데이터를 덮어쓰지 않는지까지 확인하는 케이스를 추가해 두는 편이 안전합니다.Also applies to: 147-166
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@data/src/test/java/com/threegap/bitnagil/data/routine/repositoryImpl/RoutineRepositoryImplTest.kt` around lines 48 - 80, Add a concurrent-subscription test that verifies observeWeeklyRoutines handles two simultaneous collectors for different date ranges without clobbering cache/lastFetchRange: create the repository, set remoteDataSource.scheduleResponse appropriately for each range, launch two concurrent collectors (e.g., via runTest + async/launch) that subscribe to observeWeeklyRoutines("2024-01-01","2024-01-07") and observeWeeklyRoutines("2024-01-08","2024-01-14") and keep them active long enough to receive emissions, then assert both collectors received their correct RoutineSchedule results and that remoteDataSource.fetchCount reflects two fetches; this ensures lastFetchRange and the single local cache do not overwrite each other when multiple subscribers are active.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@data/src/main/java/com/threegap/bitnagil/data/routine/repositoryImpl/RoutineRepositoryImpl.kt`:
- Around line 147-150: The current refreshCache() call swallows the Result from
fetchAndSave(), so write operations
(deleteRoutine/deleteRoutineForDay/registerRoutine/editRoutine) can report
success while the subsequent refetch fails and leaves local SSOT stale or
overwrites optimistic changes; update those callers to propagate fetchAndSave()
failures (i.e., await and return a failure Result when fetchAndSave fails) or,
alternatively, modify refreshCache()/fetchAndSave() to reapply pending
optimistic changes after a successful sync; specifically locate refreshCache(),
fetchAndSave(), and the write methods
deleteRoutine/deleteRoutineForDay/registerRoutine/editRoutine and either (A)
surface the Result from fetchAndSave() up to the write methods and return an
error when it fails, or (B) have refreshCache() call fetchAndSave(), check its
Result, and on success reapply any pending optimistic toggles before returning
so the local SSOT remains consistent.
---
Duplicate comments:
In
`@data/src/main/java/com/threegap/bitnagil/data/routine/repositoryImpl/RoutineRepositoryImpl.kt`:
- Around line 64-72: The current implementation uses a single global cache and
lastFetchRange, so concurrent subscriptions for different (startDate,endDate)
ranges overwrite each other; change to a per-range cache/flow and per-range
fetch metadata instead. Replace the single
routineLocalDataSource.routineSchedule / lastFetchRange usage with a map keyed
by Pair(startDate,endDate) (e.g., Map<Pair<String,String>,
MutableStateFlow<RoutineSchedule?>> or store per-range entries inside
RoutineLocalDataSource), update observeWeeklyRoutines to getOrCreate the flow
for the requested range rather than clearing a global cache, have
fetchAndSave(startDate,endDate) write only to that range’s entry, and ensure
rollback restores that specific range’s previous value (not the global
lastFetchRange); update calls to clearCache(), saveSchedule(), lastFetchRange to
operate per-range or remove global clearing to avoid cross-range interference.
- Around line 67-72: 현재 observe flow에서 fetchAndSave(...).getOrThrow()를 사용하면 초기
fetch나 onEach 복구 fetch 실패 시 예외가 throw되어 collector가 종료되므로, getOrThrow() 호출을 제거하고
실패를 예외로 던지지 않도록 변경하세요: 초기 블록과 onEach 블록 모두에서 fetchAndSave(startDate, endDate)를
runCatching/Result 처리하거나 try-catch로 감싸서 실패 시 routine 관찰을 중단시키지 않고 별도의 오류 채널(예:
routineErrorFlow 또는 SharedFlow/Channel)로 에러를 emit하거나 로깅한 뒤 계속 진행하도록 만드세요; 대상 심볼:
fetchAndSave, routineLocalDataSource.routineSchedule, observeWeeklyRoutines
(onEach 처리 부분).
---
Nitpick comments:
In
`@data/src/test/java/com/threegap/bitnagil/data/routine/repositoryImpl/RoutineRepositoryImplTest.kt`:
- Around line 48-80: Add a concurrent-subscription test that verifies
observeWeeklyRoutines handles two simultaneous collectors for different date
ranges without clobbering cache/lastFetchRange: create the repository, set
remoteDataSource.scheduleResponse appropriately for each range, launch two
concurrent collectors (e.g., via runTest + async/launch) that subscribe to
observeWeeklyRoutines("2024-01-01","2024-01-07") and
observeWeeklyRoutines("2024-01-08","2024-01-14") and keep them active long
enough to receive emissions, then assert both collectors received their correct
RoutineSchedule results and that remoteDataSource.fetchCount reflects two
fetches; this ensures lastFetchRange and the single local cache do not overwrite
each other when multiple subscribers are active.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d6bb0686-ec6c-4fb9-b93e-223e463faffc
📒 Files selected for processing (2)
data/src/main/java/com/threegap/bitnagil/data/routine/repositoryImpl/RoutineRepositoryImpl.ktdata/src/test/java/com/threegap/bitnagil/data/routine/repositoryImpl/RoutineRepositoryImplTest.kt
data/src/main/java/com/threegap/bitnagil/data/routine/repositoryImpl/RoutineRepositoryImpl.kt
Show resolved
Hide resolved
l5x5l
left a comment
There was a problem hiding this comment.
루틴 목록을 불러오는 로직과 관련된 부분 개선은 잘 진행해주신 것 같습니다!
다만 루틴 완료 토글과 관련된 debounce 작업은 UI 상호작용으로 보여 data레이어가 아닌 기존 방식처럼 ViewModel에서 처리하는 것이 오히려 책임에 더 적합하다고 생각합니다
그리고 토글 작업은 단발성으로 발생하기 때문에 리턴값이 없는 UseCase를 호출한 후 flow를 collect하여 갱신하는 방법보다는, 기존 방식대로 리턴 값이 존재하는 UseCase를 호출하여 그 결과값에 따라 에러 다이얼로그를 호출하는 것이 더 좋을 것 같습니다!
이와 관련해서 다른 의견 있으시다면 차주 중으로 회의 진행해서 결정해도 될 것 같으니 확인 부탁드려요!
요 부분은 현재 구현의 방향성이 ui에서는 토글 이벤트를 data 계층에서 전송하는 책임만을 가지고, 이를 전달받은 repository의 디바운스는 루틴 토글 이벤트를 500ms 안에 모아 하나의 api 호출로 처리하는 전략이기 때문에 data 계층이 더 적합하다고 판단했숩니다. 또한 viewmodel에서 디바운스를 처리할 경우 viewmodel은 자신이 속한 화면의 상태만 알 수 있기 때문에 여러 날짜의 루틴 변경사항(ex, 월 a루틴, 화 b루틴)을 하나로 묶는 처리가 불가능하지만, Repository는 날짜에 관계없이 모든 펜딩 변경사항의 전체 상태를 알고 있기에 해당 전략에 더 적합하다고 판단했숩니다.
지금 로직은 아래와 같은 흐름으로 진행이 됩니다
결국 탭 A, B, C가 각각 api를 호출하는것이 아닌 3개가 합쳐져서 api 요청이기에 웅답 결과를 누구의 리턴값으로 돌려줄 수 있는지에 대한 대상이 존재하지 않습니다. 따라서 리턴값 Usecase를 사용하려면 해당 구조를 포기하고 단일 요청으로 변경해야되는 문제가 존재합니다. |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@presentation/src/main/java/com/threegap/bitnagil/presentation/screen/routinelist/RoutineListViewModel.kt`:
- Around line 22-27: Add sync-error observation to RoutineListViewModel: add a
Boolean field showSyncErrorDialog to RoutineListState, implement a private
observeRoutineSyncError() in RoutineListViewModel that calls
observeRoutineSyncErrorUseCase() inside intent { repeatOnSubscription { ... } }
and collects errors to update state (set showSyncErrorDialog = true when an
error is emitted), and invoke observeRoutineSyncError() from the ViewModel
onCreate/init block so sync failures trigger the dialog; ensure you reference
the existing ContainerHost intent/repeatOnSubscription flow and update state via
the same state reducer used elsewhere.
- Around line 76-87: The observeWeeklyRoutines() flow lacks error handling so
exceptions from observeWeeklyRoutinesUseCase (e.g., from
fetchAndSave().getOrThrow()) will cancel the coroutine and leave isLoading=true;
add a .catch { e -> reduce { state.copy(isLoading = false /*, optionally set an
error flag/message */) }; /* log or report e */ } between
observeWeeklyRoutinesUseCase(... ) and .collect { ... } so errors are caught and
the UI state is reset, then continue to .collect to update routines as before.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 44432972-badd-44f3-a57a-be997c972fb4
📒 Files selected for processing (1)
presentation/src/main/java/com/threegap/bitnagil/presentation/screen/routinelist/RoutineListViewModel.kt
[ PR Content ]
루틴 데이터도 Flow 기반 ssot 방식으로 변경하고, Optimistic Update 책임을 data 계층으로 변경했습니다.
Related issue
Screenshot 📸
Work Description
RoutineLocalDataSource추가로 루틴 데이터의 SSOT(StateFlow) 구축RoutineRepositoryImpl에서debounce 배치 sync + rollback 처리 ->HomeViewModel의 책임 제거HomeViewModel/RoutineListViewModel모두 동일한 Repository Flow를 구독해 자동 갱신@IoDispatcher주입으로 테스트 가능성 확보 후 단위 테스트 8개 작성To Reviewers 📢
Summary by CodeRabbit
새로운 기능
개선 사항
버그 수정